Skip to content

cmake: option MZ_PBKDF2: export mz_crypt_pbkdf2#1011

Closed
sergio-nsk wants to merge 1 commit into
zlib-ng:developfrom
sergio-nsk:sergio-nsk/pbkdf2/1
Closed

cmake: option MZ_PBKDF2: export mz_crypt_pbkdf2#1011
sergio-nsk wants to merge 1 commit into
zlib-ng:developfrom
sergio-nsk:sergio-nsk/pbkdf2/1

Conversation

@sergio-nsk

@sergio-nsk sergio-nsk commented Jul 2, 2026

Copy link
Copy Markdown
Contributor

Let users use the function if they implement mz_crypt_hmac_* API without using CommonCrypto or OpenSSL.

@coderabbitai

coderabbitai Bot commented Jul 2, 2026 •
edited
Loading

Copy link
Copy Markdown

Review Change Stack

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 959419cd-ef95-4e3a-baff-87c834c00b26

📥 Commits

Reviewing files that changed from the base of the PR and between dfa766e and 6920763.

📒 Files selected for processing (2)
  • CMakeLists.txt
  • mz_crypt.c

Walkthrough

This PR introduces a new CMake build option MZ_PBKDF2 that controls PBKDF2 support, automatically enabling it when MZ_WZAES is on, defines HAVE_PBKDF2 accordingly, removes prior MZ_ZIP_NO_ENCRYPTION macro emission, and updates mz_crypt.c to gate mz_crypt_pbkdf2 compilation on HAVE_PBKDF2 instead of HAVE_WZAES.

Changes

PBKDF2 Build Option Wiring

Layer / File(s) Summary
Add MZ_PBKDF2 option and macro wiring
CMakeLists.txt		 Adds a new MZ_PBKDF2 option (default OFF), forces it ON when MZ_WZAES is enabled, appends -DHAVE_PBKDF2 to MINIZIP_DEF when enabled, and removes the previous -DMZ_ZIP_NO_ENCRYPTION emission logic.
Update compile guard for PBKDF2 implementation
mz_crypt.c		 Changes the preprocessor condition compiling mz_crypt_pbkdf2 from HAVE_WZAES to HAVE_PBKDF2, with no changes to the function body or signature.

Suggested reviewers: nmoinvaz

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title is concise and matches the main change: adding an MZ_PBKDF2 option to export mz_crypt_pbkdf2.
Description check ✅ Passed The description is related to the change, describing use of the function without CommonCrypto or OpenSSL.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@sergio-nsk sergio-nsk force-pushed the sergio-nsk/pbkdf2/1 branch from 11816eb to 7c9bdaf Compare July 2, 2026 06:56
@sergio-nsk sergio-nsk marked this pull request as draft July 2, 2026 06:57
coderabbitai[bot]
coderabbitai Bot reviewed Jul 2, 2026
View reviewed changes

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 2befbca7-4f07-4682-8248-6a3285714724

📥 Commits

Reviewing files that changed from the base of the PR and between 11816eb and 7c9bdaf.

📒 Files selected for processing (2)
  • mz_crypt.c
  • test/test_crypt.cc
💤 Files with no reviewable changes (1)
  • test/test_crypt.cc

Comment thread mz_crypt.c Outdated
@sergio-nsk
cmake: option MZ_PBKDF2: export mz_crypt_pbkdf2
6920763 
Let users use the function if they implement `mz_crypt_hmac_*` API
without using CommonCrypto or OpenSSL.
@sergio-nsk sergio-nsk force-pushed the sergio-nsk/pbkdf2/1 branch from 7c9bdaf to 6920763 Compare July 2, 2026 07:41
@sergio-nsk sergio-nsk changed the title crypt: export mz_crypt_pbkdf2 unconditionally cmake: option MZ_PBKDF2: export mz_crypt_pbkdf2 Jul 2, 2026
@sergio-nsk sergio-nsk force-pushed the sergio-nsk/pbkdf2/1 branch from 0dd25e0 to 6920763 Compare July 2, 2026 08:12
@sergio-nsk sergio-nsk marked this pull request as ready for review July 2, 2026 08:12
@sergio-nsk

sergio-nsk commented Jul 3, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

@coderabbitai Please update your "Walkthrough" comment,

@coderabbitai

coderabbitai Bot commented Jul 3, 2026

Copy link
Copy Markdown

@sergio-nsk, I'll regenerate the walkthrough comment now.

✅ Action performed

Summary regeneration triggered.

nmoinvaz
nmoinvaz requested changes Jul 6, 2026
View reviewed changes
Comment thread mz_crypt.c
}

#if defined(HAVE_WZAES)
#if defined(HAVE_PBKDF2)

@nmoinvaz nmoinvaz Jul 6, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's remove all CMake changes. Then do #if !define(NO_PBKDF2).

Then in our CMake, let's use set_target_properties on minizip-ng to set -D NO_PBKDF2.

I don't think this is worthy of a feature flag.

@sergio-nsk sergio-nsk Jul 7, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess you have meant a macro and a condition like this:

// Don't remove the right hand of the condition. Client apps may use the guarded function.
#if defined(HAVE_WZAES) || defined(HAVE_PBKDF2)

@nmoinvaz nmoinvaz added the build system Build system and script changes label Jul 6, 2026
@sergio-nsk sergio-nsk closed this Jul 7, 2026
@sergio-nsk

sergio-nsk commented Jul 7, 2026

Copy link
Copy Markdown
Contributor Author

I can -DHAVE_WZAES on the source file in the minizip-ng subdirectory.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@coderabbitai coderabbitai[bot] coderabbitai[bot] left review comments
@nmoinvaz nmoinvaz nmoinvaz requested changes

Assignees

No one assigned

Labels

build system Build system and script changes

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@sergio-nsk @nmoinvaz